[ZSTAC-85134] Carry cache pool capacity in KVM responses#3958
Open
zstack-robot-2 wants to merge 1 commit into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml) Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
Walkthrough新增并持久化主机缓存的物理总/可用容量与系统占用容量字段；更新 HostCacheStore 的 Inventory 与 Capacity VO 映射与查询字段；同步 KVM agent 的池响应 DTO，合并池容量字段到共享基类并加入池健康响应的标识字段。 变更内容主机缓存容量模型与 KVM Pool DTO
估计代码审查工作量🎯 4 (Complex) | ⏱️ ~45 minutes 兔子的诗
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Collaborator
|
Comment from yaohua.wu: Review: MR !9849 — ZSTAC-85134关联 MR
本 MR 是这套修复的"线协议基线"：在 跨仓一致性核查
Findings🟢 Suggestion — 文档化 PoolHealthRsp 的语义扩展
建议：要么在类上加一行注释说明 health response 也会携带最新容量；要么如果你觉得健康通道不该承担容量同步职责，则在 Coverage
Verdict: APPROVED线协议变更最小、对称、向下兼容。无阻塞性问题。 🤖 Robot Reviewer |
MatheMatrix
force-pushed
the
sync/haidong.pang/codex/ZSTAC-85134-refresh-cache-pool-capacity@@3
branch
from
738bef5 to
87ad2f5
Compare
Owner
|
Comment from haidong.pang: 已调整：PoolHealthRsp 不再继承 PoolRsp，check_pool 响应不携带容量，容量只放在会改变缓存池状态的 pool 操作响应里。used 字段也已经从协议中移除。 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.java`:
- Around line 5272-5284: The PoolRsp model in KVMAgentCommands is missing the
'used' capacity field and PoolHealthRsp should extend PoolRsp so health
responses reuse the same capacity carrier; update the PoolRsp class to add a
public Long used; then change PoolHealthRsp to extend PoolRsp (retain its public
Boolean healthy and public String reason fields) so health responses include
poolUuid/mountPoint/total/available/allocated/dirty/used; make the analogous
changes to the other duplicate definitions mentioned (the other
PoolRsp/PoolHealthRsp occurrences).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: 2e9f4772-c784-4d32-9c4b-76a367334d1f
📒 Files selected for processing (1)
plugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.java
Comment on lines
5272
to
5284
| public static class PoolRsp extends AgentResponse { | ||
| public String poolUuid; | ||
| public String mountPoint; | ||
| public Long total; | ||
| public Long available; | ||
| public Long allocated; | ||
| public Long dirty; | ||
| } | ||
|
|
||
| public static class PoolHealthRsp extends AgentResponse { | ||
| public Boolean healthy; | ||
| public String reason; | ||
| } |
There was a problem hiding this comment.
缓存池响应模型未完整对齐容量协议（缺 used 且 PoolHealthRsp 未继承 PoolRsp）。
当前实现会导致容量字段集合不完整，并且健康响应无法复用同一容量载体；这与本次“统一从响应刷新容量”的目标不一致。
建议修改
public static class PoolRsp extends AgentResponse {
public String poolUuid;
public String mountPoint;
public Long total;
+ public Long used;
public Long available;
public Long allocated;
public Long dirty;
}
-public static class PoolHealthRsp extends AgentResponse {
+public static class PoolHealthRsp extends PoolRsp {
public Boolean healthy;
public String reason;
}Also applies to: 5286-5287, 5295-5298
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.java` around lines
5272 - 5284, The PoolRsp model in KVMAgentCommands is missing the 'used'
capacity field and PoolHealthRsp should extend PoolRsp so health responses reuse
the same capacity carrier; update the PoolRsp class to add a public Long used;
then change PoolHealthRsp to extend PoolRsp (retain its public Boolean healthy
and public String reason fields) so health responses include
poolUuid/mountPoint/total/available/allocated/dirty/used; make the analogous
changes to the other duplicate definitions mentioned (the other
PoolRsp/PoolHealthRsp occurrences).
90daf61 to
298fc67
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@conf/db/upgrade/V5.5.18__schema.sql`:
- Around line 22-24: The new column definitions use unquoted identifiers and are
only inside a CREATE TABLE IF NOT EXISTS block, which won't add columns for
existing tables and may conflict with reserved words; update the migration to
wrap each column name in backticks (e.g., `totalPhysicalCapacity`,
`availablePhysicalCapacity`, `systemUsedCapacity`) and add an ALTER TABLE path
for upgrades (use ALTER TABLE ... ADD COLUMN ... with backticks) or implement a
stored procedure that checks for column existence and adds the NOT NULL columns
while backfilling historical rows safely (e.g., add with DEFAULT or add nullable
then UPDATE existing rows then ALTER to NOT NULL), referencing the CREATE TABLE
IF NOT EXISTS, ALTER TABLE and stored procedure approach in the migration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: b8258f32-b076-4959-b440-3165ba0f3d4d
⛔ Files ignored due to path filters (1)
sdk/src/main/java/org/zstack/sdk/HostCacheStoreInventory.javais excluded by!sdk/**
📒 Files selected for processing (7)
conf/db/upgrade/V5.5.18__schema.sqlheader/src/main/java/org/zstack/header/volumeCache/HostCacheStoreCapacityInventory.javaheader/src/main/java/org/zstack/header/volumeCache/HostCacheStoreCapacityVO.javaheader/src/main/java/org/zstack/header/volumeCache/HostCacheStoreCapacityVO_.javaheader/src/main/java/org/zstack/header/volumeCache/HostCacheStoreInventory.javaheader/src/main/java/org/zstack/header/volumeCache/HostCacheStoreInventoryDoc_zh_cn.groovyplugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.java
✅ Files skipped from review due to trivial changes (2)
- header/src/main/java/org/zstack/header/volumeCache/HostCacheStoreCapacityVO_.java
- header/src/main/java/org/zstack/header/volumeCache/HostCacheStoreInventoryDoc_zh_cn.groovy
Comment on lines
+22
to
+24
| `totalPhysicalCapacity` BIGINT NOT NULL DEFAULT 0, | ||
| `availablePhysicalCapacity` BIGINT NOT NULL DEFAULT 0, | ||
| `systemUsedCapacity` BIGINT NOT NULL DEFAULT 0, |
There was a problem hiding this comment.
新增列名需使用反引号包裹，且升级场景未正确处理。
根据编码规范，所有列名必须使用反引号包裹以避免 MySQL 8.0 / GreatSQL 保留关键字冲突。此外，CREATE TABLE IF NOT EXISTS 语句在表已存在时不会添加新列，升级场景需要使用 ALTER TABLE 或存储过程来处理。
建议修改：添加 ALTER TABLE 语句处理升级场景
CREATE TABLE IF NOT EXISTS `HostCacheStoreCapacityVO` (
`uuid` VARCHAR(32) NOT NULL,
`totalCapacity` BIGINT NOT NULL DEFAULT 0,
`availableCapacity` BIGINT NOT NULL DEFAULT 0,
- `totalPhysicalCapacity` BIGINT NOT NULL DEFAULT 0,
- `availablePhysicalCapacity` BIGINT NOT NULL DEFAULT 0,
- `systemUsedCapacity` BIGINT NOT NULL DEFAULT 0,
+ `totalPhysicalCapacity` BIGINT NOT NULL DEFAULT 0,
+ `availablePhysicalCapacity` BIGINT NOT NULL DEFAULT 0,
+ `systemUsedCapacity` BIGINT NOT NULL DEFAULT 0,
PRIMARY KEY (`uuid`),
...
) ENGINE = InnoDB DEFAULT CHARSET = utf8;
+-- Handle upgrade scenario: add new columns if table already exists
+CALL AddColumnIfNotExists('zstack', 'HostCacheStoreCapacityVO', 'totalPhysicalCapacity', 'BIGINT NOT NULL DEFAULT 0');
+CALL AddColumnIfNotExists('zstack', 'HostCacheStoreCapacityVO', 'availablePhysicalCapacity', 'BIGINT NOT NULL DEFAULT 0');
+CALL AddColumnIfNotExists('zstack', 'HostCacheStoreCapacityVO', 'systemUsedCapacity', 'BIGINT NOT NULL DEFAULT 0');As per coding guidelines: "所有表名和列名必须使用反引号包裹" 以及 "When NOT NULL exists, must use stored procedure or other functions to process historical data"。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@conf/db/upgrade/V5.5.18__schema.sql` around lines 22 - 24, The new column
definitions use unquoted identifiers and are only inside a CREATE TABLE IF NOT
EXISTS block, which won't add columns for existing tables and may conflict with
reserved words; update the migration to wrap each column name in backticks
(e.g., `totalPhysicalCapacity`, `availablePhysicalCapacity`,
`systemUsedCapacity`) and add an ALTER TABLE path for upgrades (use ALTER TABLE
... ADD COLUMN ... with backticks) or implement a stored procedure that checks
for column existence and adds the NOT NULL columns while backfilling historical
rows safely (e.g., add with DEFAULT or add nullable then UPDATE existing rows
then ALTER to NOT NULL), referencing the CREATE TABLE IF NOT EXISTS, ALTER TABLE
and stored procedure approach in the migration.
Collaborator
Author
There was a problem hiding this comment.
Comment from haidong.pang:
未发布功能，不考虑该场景
There was a problem hiding this comment.
@zstack-robot-2，明白了，既然 HostCacheStoreCapacityVO 是尚未发布的新表，不存在需要升级的历史数据，CREATE TABLE IF NOT EXISTS 方式完全可行，无需额外的 ALTER TABLE 升级路径。感谢说明！
(＾・ω・＾ )
🧠 Learnings used
Learnt from: zstack-robot-2
Repo: MatheMatrix/zstack PR: 3416
File: conf/db/upgrade/V5.5.12__schema.sql:24-35
Timestamp: 2026-03-05T02:13:23.339Z
Learning: In ZStack, the database schema name is fixed as 'zstack'. For SQL upgrade scripts under conf/db/upgrade, continue using TABLE_SCHEMA = 'zstack' and reference tables as zstack.`UsedIpVO` (i.e., qualify with the schema). Do not replace with DATABASE() or remove the schema qualifier for portability. This pattern can be assumed across upgrade scripts in this directory.
Learnt from: zstack-robot-2
Repo: MatheMatrix/zstack PR: 3891
File: conf/db/upgrade/V5.5.22__schema.sql:28-31
Timestamp: 2026-05-07T09:44:01.429Z
Learning: In ZStack MySQL upgrade scripts under conf/db/upgrade/*.sql, if a table already contains a TIMESTAMP column defined with ON UPDATE CURRENT_TIMESTAMP (e.g., lastOpDate), then any second TIMESTAMP column (e.g., createDate) must NOT use DEFAULT CURRENT_TIMESTAMP. This repository’s CI/validated environment uses MySQL 5.7 with explicit_defaults_for_timestamp=OFF, which can trigger MySQL Error 1293 (only one TIMESTAMP column with CURRENT_TIMESTAMP in DEFAULT/ON UPDATE). Use a non-CURRENT_TIMESTAMP sentinel default such as '2000-01-01 00:00:00' (and typically keep it NOT NULL), and rely on ModelServiceRefVO.PrePersist in Java to set the real creation timestamp for new JPA-managed rows.
Add cache pool capacity fields to mutating pool-related KVM agent response DTOs so MN can refresh capacity from operation responses. Resolves: ZSTAC-85134 Change-Id: I04b8c898a8fcc213f025a4db848e2f4cb97689f5
MatheMatrix
force-pushed
the
sync/haidong.pang/codex/ZSTAC-85134-refresh-cache-pool-capacity@@3
branch
from
298fc67 to
26925a6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
为缓存池相关 KVM agent response 统一补充容量字段，使 PoolRsp、PoolHealthRsp、PoolCapacityRsp、GCPoolRsp 都能携带当前缓存池容量信息，供 MN 在业务响应中直接刷新容量。
sync from gitlab !9849